feat: agent execution resilience - file conflicts, verification recovery, observability - #342
Conversation
…on recovery, observability
Three improvements to agent execution reliability:
1. File conflict handling (executor + planner):
- file_create now falls back to edit when file exists with different content
- Returns no-op success when file exists with identical content
- Planner prompt warns about existing files to guide LLM toward file_edit
2. Verification recovery (agent):
- Separate tracking of verification failures vs step execution failures
- Early abort after MAX_CONSECUTIVE_VERIFICATION_FAILURES (3) with blocker
- Incremental verification now captures full ruff output (verbose=True)
for better self-correction context
- Enhanced error details passed to self-correction attempts
3. Observability (gates + events):
- Structured ruff error parsing into {file, line, col, code, message} dicts
- GateCheck.detailed_errors field for programmatic access
- GateResult.get_error_summary() and get_errors_by_file() methods
- verification_failed events now include gate names, error count, and details
Tests: 15 new tests (TDD), 1003/1003 core tests pass, 0 regressions.
WalkthroughEnhances verification failure handling with an abort threshold and richer failure payloads, adds Ruff error parsing and observability, makes file_create gracefully fall back to edit when files exist, augments planner prompts with existing-files guidance, and adds tests covering these behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant Agent
participant PlanExecutor
participant GateChecks
participant Blocker
Agent->>PlanExecutor: execute_plan()
PlanExecutor->>PlanExecutor: init consecutive_verification_failures = 0
loop plan steps
PlanExecutor->>GateChecks: run incremental verification (verbose=True)
alt passed
GateChecks-->>PlanExecutor: gate_result.passed = true
PlanExecutor->>PlanExecutor: consecutive_verification_failures = 0
else failed
GateChecks-->>PlanExecutor: gate_result.passed = false, detailed_errors[]
PlanExecutor->>PlanExecutor: consecutive_verification_failures += 1
PlanExecutor->>Agent: emit verification_failed (error_names, error_count, error_details)
alt consecutive_verification_failures >= MAX_CONSECUTIVE_VERIFICATION_FAILURES
PlanExecutor->>Blocker: create_blocker(with failure context)
PlanExecutor->>Agent: emit execution_aborted
PlanExecutor-->>Agent: abort execution
else
PlanExecutor->>PlanExecutor: attempt self-correction and continue
end
end
end
sequenceDiagram
participant Executor
participant FileSystem
participant LLM
participant Storage
Executor->>FileSystem: check if file exists (path)
alt file does not exist
Executor->>FileSystem: write new file (create)
Executor->>Storage: record FileChange (create)
Executor-->>Executor: return SUCCESS
else file exists
Executor->>FileSystem: read existing content
alt content identical
Executor-->>Executor: return SUCCESS (no-op)
else content differs
alt dry_run
Executor-->>Executor: return SUCCESS (dry-run message)
else normal
Executor->>LLM: generate/confirm new content
LLM-->>Executor: updated content
Executor->>FileSystem: write updated content (edit)
Executor->>Storage: record FileChange (edit, old_content)
Executor-->>Executor: return SUCCESS
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Code Review - PR #342: Agent Execution ResilienceI've reviewed the changes for agent execution resilience improvements. Overall, this is a well-designed enhancement that addresses real pain points in agent execution. Strengths
Code Quality Observations1. Ruff Error Parsing (gates.py:27) 2. Error Detail Truncation (agent.py:641, 654) 3. consecutive_verification_failures Reset (agent.py:695) 4. File Content Comparison (executor.py:302) Missing Test Coverage
RecommendationsHigh Priority:
Medium Priority:
Low Priority:
SummaryThis is high-quality work following the repository's principles. Recommendation: Approve with minor suggestions. The high-priority items should be addressed before merge. |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
Abort agent execution after 3 consecutive verification failures, emit structured verification events, and treat
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
codeframe/core/agent.py (1)
622-625:⚠️ Potential issue | 🟠 MajorReset consecutive verification failures after a successful incremental verification.
Right now the counter only resets on self-correction success, so a later clean verification still leaves the counter > 0 and can trigger a premature abort.✅ Suggested fix
- if gate_result and not gate_result.passed: + if gate_result and gate_result.passed: + consecutive_verification_failures = 0 + elif gate_result and not gate_result.passed: # Try to fix lint issues automatically (works for style, not syntax)
🤖 Fix all issues with AI agents
In `@codeframe/core/agent.py`:
- Around line 700-711: The early-abort path checks
consecutive_verification_failures and calls
self._create_blocker_from_failure(step, current_result) but that helper can
return without creating a blocker, leaving the step in EXECUTING; ensure a
blocker is always created on this abort: call _create_blocker_from_failure and
if it returns a falsy/None value then explicitly create or mark the blocker
(e.g. call a guaranteed blocker creation routine or set the step/state to
BLOCKED and emit the "execution_aborted" event) so that the abort always forces
an actual blocker regardless of TECHNICAL_FIX/RESOLVE_AUTONOMOUSLY paths;
reference the symbols consecutive_verification_failures,
MAX_CONSECUTIVE_VERIFICATION_FAILURES, self._create_blocker_from_failure(step,
current_result), self._emit_event("execution_aborted", ...), and step to
implement the check-and-force behavior.
In `@codeframe/core/gates.py`:
- Around line 121-151: Update the GATES_COMPLETED event payload to include the
structured diagnostics produced by get_error_summary() and get_errors_by_file(),
and add a "suggestions" field per gate (e.g., suggested fixes or actionable
hints); specifically, when emitting the GATES_COMPLETED event for each Gate
instance (use the existing checks list / Gate object), call get_error_summary()
and get_errors_by_file() and include their outputs as "error_summary" (string)
and "errors_by_file" (dict) in the payload along with the existing gate name and
pass/fail status, and add a "suggestions" array populated from any
Check.suggestions or a synthesized suggestion list so consumers can display
remediation steps.
- Around line 27-51: The regex _RUFF_ERROR_PATTERN used by _parse_ruff_errors
only allows a single uppercase letter before the digits and thus drops
multi-letter rule codes like ANN401 or PLR2004; update the pattern to accept one
or more uppercase letters before the digits (e.g. change ([A-Z]\d+) to
([A-Z]+\d+)) so _parse_ruff_errors will correctly capture multi-letter rule
codes while keeping the same capture groups for file, line, col, code, and
message.
| def get_error_summary(self) -> str: | ||
| """Format all errors into a readable multi-line string. | ||
|
|
||
| Returns: | ||
| Newline-separated string of all structured errors from failed checks, | ||
| or empty string if no errors. | ||
| """ | ||
| lines = [] | ||
| for check in self.checks: | ||
| if check.detailed_errors: | ||
| for err in check.detailed_errors: | ||
| lines.append( | ||
| f"{err['file']}:{err['line']}:{err['col']}: " | ||
| f"{err['code']} {err['message']}" | ||
| ) | ||
| return "\n".join(lines) | ||
|
|
||
| def get_errors_by_file(self) -> dict[str, list[str]]: | ||
| """Group error messages by file path. | ||
|
|
||
| Returns: | ||
| Dict mapping file paths to lists of formatted error strings. | ||
| """ | ||
| by_file: dict[str, list[str]] = {} | ||
| for check in self.checks: | ||
| if check.detailed_errors: | ||
| for err in check.detailed_errors: | ||
| file_path = err["file"] | ||
| msg = f"{err['code']} {err['message']} (line {err['line']})" | ||
| by_file.setdefault(file_path, []).append(msg) | ||
| return by_file |
There was a problem hiding this comment.
Emit structured gate error details in the GATES_COMPLETED event.
You already added get_error_summary() / get_errors_by_file(); surfacing those (and a suggestions field) in the gate completion payload is required for diagnostics.
✅ Suggested fix
- events.emit_for_workspace(
- workspace,
- events.EventType.GATES_COMPLETED,
- {
- "passed": passed,
- "summary": result.summary,
- "checks": [{"name": c.name, "status": c.status.value} for c in checks],
- },
- print_event=True,
- )
+ payload = {
+ "passed": passed,
+ "summary": result.summary,
+ "checks": [{"name": c.name, "status": c.status.value} for c in checks],
+ }
+ if not passed:
+ payload["error_summary"] = result.get_error_summary()
+ payload["errors_by_file"] = result.get_errors_by_file()
+ payload["suggestions"] = [] # populate when available
+
+ events.emit_for_workspace(
+ workspace,
+ events.EventType.GATES_COMPLETED,
+ payload,
+ print_event=True,
+ )As per coding guidelines: "Modules handling verification gates (ruff, pytest, BUILD checks) must emit detailed diagnostic events including gate name, passed/failed status, error messages, and suggestions for fixes."
🤖 Prompt for AI Agents
In `@codeframe/core/gates.py` around lines 121 - 151, Update the GATES_COMPLETED
event payload to include the structured diagnostics produced by
get_error_summary() and get_errors_by_file(), and add a "suggestions" field per
gate (e.g., suggested fixes or actionable hints); specifically, when emitting
the GATES_COMPLETED event for each Gate instance (use the existing checks list /
Gate object), call get_error_summary() and get_errors_by_file() and include
their outputs as "error_summary" (string) and "errors_by_file" (dict) in the
payload along with the existing gate name and pass/fail status, and add a
"suggestions" array populated from any Check.suggestions or a synthesized
suggestion list so consumers can display remediation steps.
…etter ruff codes Three fixes from code review: 1. Reset consecutive_verification_failures on clean verification pass, not just on self-correction success. Prevents premature abort after fail → pass → fail → fail sequences. 2. Abort path now calls blockers.create directly instead of _create_blocker_from_failure, which could silently return for RESOLVE_AUTONOMOUSLY/TECHNICAL_FIX classifications. Abort must always produce a blocker. 3. Ruff error regex now accepts multi-letter rule codes (ANN401, PLR2004, SIM118, UP035) by changing [A-Z] to [A-Z]+. Declined: enriching GATES_COMPLETED event payload with structured diagnostics and suggestions field — YAGNI, the methods exist for programmatic access.
Updated Code Review - PR #342I've reviewed the changes again. This is a solid improvement to agent execution resilience. My previous review covered the main points, and I see CodeRabbit has also provided detailed feedback. Here are some additional observations: Critical Issue from CodeRabbit ReviewRegex Pattern for Multi-Letter Rule Codes (gates.py:218) CodeRabbit correctly identified that the regex pattern only matches single-letter prefixes. This will fail for common ruff rules like ANN401, PLR2004, SIM118, and UP035. Fix needed: Change [A-Z]\d+ to [A-Z]+\d+ in the regex pattern on line 218. I notice there's already a test for this in test_gates_observability.py:605 which is excellent - but the regex needs to be updated to make that test pass. Additional Observations1. Verification Counter Reset (agent.py:625-627) Good - the counter now properly resets on successful verification. This prevents the scenario: fail → pass → fail → fail from incorrectly triggering abort. 2. Error Detail Truncation Consistency The truncation strategy has two levels:
This is reasonable but worth documenting in code comments. 3. File Conflict Fallback (executor.py:302) The .strip() comparison for content identity is pragmatic but could hide meaningful whitespace differences. Consider adding a verbose log when strip() matches but raw content differs. 4. Early Abort Path (agent.py:701-711) The forced blocker creation bypassing LLM classification is the right approach. This ensures we always get a blocker on abort, not a silent RESOLVE_AUTONOMOUSLY classification. Test Coverage AssessmentStrong test coverage with 15 new tests. The tests use both integration-style checks and source inspection, which is appropriate for TDD verification. All high-priority scenarios are covered. Recommendations PriorityMust fix before merge:
Should consider: Nice to have: SummaryThis PR delivers meaningful improvements to agent reliability. The regex bug is the only blocking issue. Once fixed, this is ready to merge. Excellent work on the separation of concerns between verification failures and step execution failures - this architectural choice will pay dividends. |
1 participant
Summary
Improves agent execution reliability with three targeted enhancements:
file_createnow falls back to edit when a file already exists (instead of failing), and the planner prompt warns the LLM about existing files to guide correct operation choice{file, line, col, code, message}dicts, newGateResult.get_error_summary()andget_errors_by_file()methods, and enrichedverification_failedSSE events with gate names, error counts, and detailsTest plan
Summary by CodeRabbit
New Features
Bug Fixes
Tests